-
Notifications
You must be signed in to change notification settings - Fork 453
feat: deploy from scratch scripts #1661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: deploy from scratch scripts #1661
Conversation
0bbf516 to
4c7842c
Compare
4aaaebb to
1e9a701
Compare
nadir-akhtar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't have the time to meticulously review the protocol-from-scratch scripts directory unfortunately, but overall LGTM. Markdown was helpful to reference against.
Seems like CI is failing for a test though. Made a note of possible cause
|
|
||
| import "./1-deployMultichainDeployer.s.sol"; | ||
| import {MultisigBuilder} from "zeus-templates/templates/MultisigBuilder.sol"; | ||
| import {IMultisig} from "script/releases/MultisigDeployLib.sol"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems unnecessary as this import is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used when modifying and retrieving threshold
|
|
||
| function testScript() public virtual override { | ||
| // 1. Deploy destination chain contracts | ||
| DeployMultichainDeployer.testScript(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that, when the contracts are already deployed, the threshold is 3 but the parent test here checks for threshold = 1, causing CI to fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to pass
| // ReleaseManager is not initializable | ||
| } | ||
|
|
||
| // KeyRegistrar and PermissionController are not initializable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: clean up these comments, they're a bit confusing (above we say PermissionController is initalizable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| { | ||
| /// core/ | ||
|
|
||
| // Permission controller has no constructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: put this in the permissions block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c3448b3
into
release/destination-genesis-scripts

Motivation:
We want deploy from scratch scripts for preprod-hoodi.
Modifications:
v1.6.0-protocol-from-scratchscriptMultisigDeployerLibto dedupe all the Multisig calls we were makingScripts.mdfile that describes the available scriptsResult:
Clear deploy from scratch